shelter: show icon in header and footer (DEV-1177) #801
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request implements the display of the Better Angels logo in the header and footer of the Shelter web application. Class diagram showing updated component structureclassDiagram
class Header {
+render() ReactElement
}
class Footer {
+render() ReactElement
}
class BetterangelsLogoIcon {
+render() ReactElement
}
Header --> BetterangelsLogoIcon : uses
Footer --> BetterangelsLogoIcon : uses
note for BetterangelsLogoIcon "New SVG icon component"
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
tglaz
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @tglaz - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return <header className={parentCss}>hello shelter-app</header>; | ||
| return ( | ||
| <header className={parentCss}> | ||
| <div className="flex items-center"> |
There was a problem hiding this comment.
suggestion: Consider extracting the logo and text into a shared component to avoid duplication with the footer
This would make the codebase more maintainable and ensure consistent branding across components.
Suggested implementation:
import { ReactElement } from 'react';
import { BrandLogo } from '../components/BrandLogo'; <BrandLogo size="small" />You'll need to:
- Create a new file
apps/shelter-web/src/app/components/BrandLogo.tsxwith this content:
import { BetterangelsLogoIcon } from '@monorepo/react/icons';
type BrandLogoProps = {
size?: 'small' | 'large';
};
export function BrandLogo({ size = 'small' }: BrandLogoProps): ReactElement {
const logoHeight = size === 'small' ? 'h-4' : 'h-8';
return (
<div className="flex items-center">
<BetterangelsLogoIcon className={`${logoHeight} text-brand-yellow`} />
<div className="text-white flex ml-2 text-sm">
<div className="font-normal">Shelters</div>
<div className="font-semibold">LA</div>
</div>
</div>
);
}- Update the footer component to use:
<BrandLogo size="large" />This creates a reusable component that maintains consistency while allowing for different sizes in different contexts.
| <div className="font-semibold">LA</div> | ||
| </div> | ||
| </div> | ||
| <div className="mt-6 border-t-[0.5px] border-white pt-6 flex justify-end"> |
There was a problem hiding this comment.
suggestion: Consider using a standard border width instead of 0.5px for better cross-browser consistency
Sub-pixel values can render inconsistently across different browsers. Consider using 1px or a standard Tailwind border width class.
Suggested change
| <div className="mt-6 border-t-[0.5px] border-white pt-6 flex justify-end"> | |
| <div className="mt-6 border-t border-white pt-6 flex justify-end"> |
tglaz
force-pushed
the
DEV-1177/shelter-logo-header-footer
branch
3 times, most recently
from
8826e58 to
eed4494
Compare
tglaz
force-pushed
the
DEV-1177/shelter-logo-header-footer
branch
from
eed4494 to
bc631fd
Compare
johnr54321
approved these changes
Dec 23, 2024
There was a problem hiding this comment.
Thanks @tglaz ! Definitely looks better with the logo. When we do the official header and footer tickets with the designs we'll do any design clean up.
Davit-BetterAngels
approved these changes
Dec 23, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a shelter-web user, I want to recognize the app by its logo
https://betterangels.atlassian.net/browse/DEV-1177
preview at: https://shelter.dev.betterangels.la/logo
Notes:
SheltersLAtext to match branding (in separate ticket)Summary by Sourcery
New Features: